Undo a shrinking in-place realloc in the reverse sweep. - #1936
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| // being overwritten by value restores. Returns the possibly-moved pointer. | ||
| inline void* reverse_realloc(void* ptr, size_t oldBytes, size_t newBytes, | ||
| bool zeroGrownTail) { | ||
| ptr = ::realloc(ptr, oldBytes); |
There was a problem hiding this comment.
warning: 'ptr' may be set to null if 'realloc' fails, which may result in a leak of the original buffer [bugprone-suspicious-realloc-usage]
ptr = ::realloc(ptr, oldBytes);
^| // being overwritten by value restores. Returns the possibly-moved pointer. | ||
| inline void* reverse_realloc(void* ptr, size_t oldBytes, size_t newBytes, | ||
| bool zeroGrownTail) { | ||
| ptr = ::realloc(ptr, oldBytes); |
There was a problem hiding this comment.
warning: assigning newly created 'gsl::owner<>' to non-owner 'void *' [cppcoreguidelines-owning-memory]
ptr = ::realloc(ptr, oldBytes);
^| // being overwritten by value restores. Returns the possibly-moved pointer. | ||
| inline void* reverse_realloc(void* ptr, size_t oldBytes, size_t newBytes, | ||
| bool zeroGrownTail) { | ||
| ptr = ::realloc(ptr, oldBytes); |
There was a problem hiding this comment.
warning: calling legacy resource function without passing a 'gsl::owner<>' [cppcoreguidelines-owning-memory]
ptr = ::realloc(ptr, oldBytes);
^| // being overwritten by value restores. Returns the possibly-moved pointer. | ||
| inline void* reverse_realloc(void* ptr, size_t oldBytes, size_t newBytes, | ||
| bool zeroGrownTail) { | ||
| ptr = ::realloc(ptr, oldBytes); |
There was a problem hiding this comment.
warning: do not manage memory manually; consider std::vector or std::string [cppcoreguidelines-no-malloc]
ptr = ::realloc(ptr, oldBytes);
^| bool zeroGrownTail) { | ||
| ptr = ::realloc(ptr, oldBytes); | ||
| if (zeroGrownTail && ptr && oldBytes > newBytes) | ||
| ::memset(static_cast<char*>(ptr) + newBytes, 0, oldBytes - newBytes); |
There was a problem hiding this comment.
warning: do not use pointer arithmetic [cppcoreguidelines-pro-bounds-pointer-arithmetic]
::memset(static_cast<char*>(ptr) + newBytes, 0, oldBytes - newBytes);
^| /// Maps a differentiated pointer variable to the `size_t` shadow that holds | ||
| /// its current allocation size in bytes. Populated at malloc/calloc/realloc | ||
| /// so an in-place realloc can restore the pre-realloc size in reverse. | ||
| llvm::DenseMap<const clang::VarDecl*, clang::VarDecl*> m_PointerAllocSize; |
There was a problem hiding this comment.
warning: no header providing "llvm::DenseMap" is directly included [misc-include-cleaner]
include/clad/Differentiator/ReverseModeVisitor.h:38:
- #include <memory>
+ #include <llvm/ADT/DenseMap.h>
+ #include <memory>
e7e2540 to
66dcaaa
Compare
| // it instead of duplicating the matcher. | ||
| // NOLINTNEXTLINE(cppcoreguidelines-pro-type-const-cast) | ||
| AllocCallInfo ac = | ||
| AllocCallInfo::recognize(const_cast<Expr*>(BO->getRHS())); |
There was a problem hiding this comment.
warning: redundant explicit casting to the same type 'Expr *' as the sub-expression, remove this casting [readability-redundant-casting]
| AllocCallInfo::recognize(const_cast<Expr*>(BO->getRHS())); | |
| AllocCallInfo::recognize(BO->getRHS()); |
Additional context
llvm/include/clang/AST/Expr.h:3969: source type originates from the invocation of this method
Expr *getRHS() const { return cast<Expr>(SubExprs[RHS]); }
^
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Since #1925 an in-place `p = realloc(p, n)` keeps the reallocated pointer for both p and its adjoint instead of saving and restoring it, because realloc frees the old block and a saved pointer would dangle. That is correct only when the buffer grows or keeps its size: a shrinking realloc leaves the reverse sweep addressing a buffer smaller than the indices the forward pass wrote, reading past its end. Track each differentiated pointer's allocation size in a size_t shadow, set at malloc/calloc/realloc. At an in-place realloc, save the pre-realloc size in the forward pass and, in the reverse sweep, call clad::reverse_realloc to resize both the primal and adjoint buffers back to it so the pre-realloc accesses stay in bounds; the adjoint's re-grown tail is zeroed so fresh derivatives start at 0. Without a tracked size the previous keep-the-pointer behaviour still applies. Add Gradient/ReallocShrink.C, which is Memcheck-clean under the valgrind row only with this change; update Gradient/Pointers.C for the shadows.
66dcaaa to
01add83
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
1 participant
Since #1925 an in-place
p = realloc(p, n)keeps the reallocated pointer for both p and its adjoint instead of saving and restoring it, because realloc frees the old block and a saved pointer would dangle. That is correct only when the buffer grows or keeps its size: a shrinking realloc leaves the reverse sweep addressing a buffer smaller than the indices the forward pass wrote, reading past its end.Track each differentiated pointer's allocation size in a size_t shadow, set at malloc/calloc/realloc. At an in-place realloc, save the pre-realloc size in the forward pass and, in the reverse sweep, call clad::reverse_realloc to resize both the primal and adjoint buffers back to it so the pre-realloc accesses stay in bounds; the adjoint's re-grown tail is zeroed so fresh derivatives start at 0. Without a tracked size the previous keep-the-pointer behaviour still applies.
Add Gradient/ReallocShrink.C, which is Memcheck-clean under the valgrind row only with this change; update Gradient/Pointers.C for the shadows.